New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpf: add -nostdinc and a few more misc compilation options #11205
Conversation
test-me-please |
test-me-please |
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor nits but changes LGTM.
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like removal of get_hash_recalc()
got split between the two patches.
Otherwise looks good, but second patch is huge. Did all of those fixes came out of the new warnings at compile time?
test-me-please |
Yeah not all of it, for example, the event output msg refactoring touches all other ones as well to have a consistent initialiser. Hmm, I can split if off from the 2nd one to shrink it further. |
If you don't mind too much, that would help with the review. |
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
test-me-please |
restart-ginkgo |
|
test-me-please |
test-focus K8sService.* |
|
restart-ginkgo |
1 similar comment
restart-ginkgo |
@borkmann it seems Cilium was in CrashLoopback mode. Doesn't seem to be a flake |
Yep, saw it, will keep working on it in the evening today. |
Commit 2097a89c2e990169b964fdf716b9fbc36f6ab3bd does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
test-focus K8sService.* |
test-me-please |
1 similar comment
test-me-please |
provision error |
test-me-please |
4.19:
|
test-with-kernel |
Depending on the NICs hash, we might end up recalculating it from the kernel flow dissector to get a higher quality L4 hash on the skb instead of L3 one, for example. We used to do this mainly for the skb->hash based service LB, but nowadays all occurences in the code are for cilium monitor debug output where it's used as a heuristic to see that the event messages belong to the same flow. Hubble side doesn't do anything with the hash. Whether L3 or L4 hash doesn't really matter too much here, so get rid of these calls. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Prep work and general refactoring for enabling stricter compilation options closer to the kernel. In order to avoid warnings from the -Wdeclaration-after-statement, make use of compound literals where adequate. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Fetch the get_netns_cookie() only once given it's an atomic op and we already have it locally when we check for hostns. Also pass in svc directly into session affinity APIs to simplify them a bit and move the multiple defined(ENABLE_SESSION_AFFINITY) && defined(BPF_HAVE_NETNS_COOKIE) out of the bpf_sock.c code by letting compiler do most of the work to optimize away the constants - we add *_{netns,addr}() api variants and for the former we can place the BPF_HAVE_NETNS_COOKIE ifdef into lb.h header to keep the code flow more readable in bpf_sock.c. At the same time this also fixes couple of -Wdeclaration-after-statement warnings. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
test-focus K8sService.* |
Few random minor changes over the place to fix up warnings in order to enable stricter compile checks in the subsequent patch. This also fixes up some whitespace oddities in nodeport code that slipped into our tree. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Given bpf_overlay is similarly serving BPF NodePort like bpf_netdev does, improve coverage so we can easier spot any build issues. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
We want to have our code as close to usual kernel conventions as possible to ease review, so add a few more switches for clang where we emit useful warnings in our code if it's not the case, that is, -std=gnu89 and -Wdeclaration-after-statement. We also use quite a number of shadow declarations where they are really useless; I'm not sure whether clang ends up allocating new stack space for them; in any case lets add -Wshadow given the current locations are fixed now. Also, given our headers are all self-contained, add -nostdinc to fully /guarantee/ that clang does not search the standard include paths anymore. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
test-me-please |
See commit msg.